ext/{lexbor,uri}: Fix build/install interface #20506
Conversation
- Currently, ext/uri API is designed in a way that no 3rd party uriparser headers are needed to use ext/uri headers in some other extension. - ext/lexbor now also installs all 3rd party ext/lexbor/lexbor headers. They are currently needed by ext/dom and ext/uri. - Added lexbor include flag to php-config script. - The ext/uri/uriparser/include can be (currently) only specified for the ext/uri sources and not globally for all other extensions. - Creating redundant ext/lexbor/lexbor/css/tokenizer directory at configure step removed (this was probably used in some older lexbor version).
|
LGTM, although I can barely do a meaningful assessment. cc @TimWolla |
|
I can't meaningfully comment on this either and trust your autoconf knowledge on this. However I believe this is too late for PHP 8.5 / just a cleanup, not a bugfix, no? |
For this I agree it would make sense to make the return type of |
|
Yes, this is a bugfix for PHP 8.5: cd ext/dom
phpize
./configure
makeAnd to enable creating a PHP extension which will use the |
Can this be done for PHP 8.5? Then this PR can be also modified and simplified. Because adding -Iext/lexbor include flag would be best to avoid for now then. |
No. That would be an API change which definitely is unacceptable after the release. But forward-declaring the |
|
Let's simplify this a bit then and I'll remove those -I changes. If someone will already create a PHP extension which will use ext/uri headers, they can add the -Iext/lexbor manually in the config.{m4,w32} for PHP-8.5 like it's done in ext/dom and ext/uri now in PHP-8.5 branch. Adding new global include flags to php-config on both build systems is definitely something that I'd avoid here. And in PHP-8.6 we'll change it. |
3 participants
As much as I dislike adding another global include flag to php-config script or to install even more headers, this is now needed to fix then also, I believe.
Changes in the PR:
Currently, ext/uri API is designed in a way that no 3rd party uriparser headers are needed to use ext/uri headers in some other extension.
ext/lexbor now also installs all 3rd party ext/lexbor/lexbor headers. They are currently needed by ext/dom and ext/uri.
Added lexbor include flag to php-config script.
The ext/uri/uriparser/include can be (currently) only specified for the ext/uri sources and not globally for all other extensions.
Creating redundant ext/lexbor/lexbor/css/tokenizer directory at configure step removed (this was probably used in some older lexbor version).
In the future it would be much cooler to go similar to how the ext/pcre/pcre2lib is done regarding the headers. Or in current case, how the ext/uri is done - like a perfect example of using 3rd party library, having it bundled (optionally) but no uriparser headers need to be installed to use PHP C API.
BTW, to use PHP, a list of all include flags is getting extremely long. This is completely redundant I believe and could be simplified to only
includes="-I$include_dir"at some point if there is some goal to have a nice PHP C API:Edit: This is done now only due to the
ext/uri/uri_parser_whatwg.hwhich includes thelexbor/url/url.hand this is a public header. If this header could be somehow refactored, then ext/lexbor include flag could be most likely removed from the php-config scripts. So only ext/dom has customly could have an -Iext/lexbor flag.